Implement assets webpack plugin #410

codayblue · 2018-10-14T22:27:06Z

This should solve issue #408 because it implements the plugin that was requested to be looked into.

The changes that have happened by implementing this plugin is that the keys might not be in the file for a given entry. So lets say we have a main entry point that has JavaScript but no CSS, the CSS key will not be under that entry point. The other issue is that when a file type only has one file of a given file type it will not be in an array in the entrypoints.json file it will just be a string value linked to the property.

Lyrkan

Hi @codayblue,

Thank you for working on that :)

In addition to all my remarks could you also remove the code that isn't used anymore (for instance lib/webpack/entry-files-manifest-plugin.js)?

Lyrkan · 2018-10-15T12:32:26Z

.eslintrc.js

@@ -52,7 +52,11 @@ module.exports = {
            }
        ],
        "node/no-unpublished-bin": "error",
-        "node/no-unpublished-require": "error",
+        "node/no-unpublished-require": ["error", {


This is really odd that assets-webpack-plugin should be excluded there... I'm really wondering if this is not the sign that something else is wrong.

Where should I look? This was failing on my local and in Travis. When I did some looking this was the suggested answer from google. I dug a little more but found no other path. Just wondering what suggestions you may have.

That's because you added assets-webpack-plugin as a dev dependency.

Since people installing Encore in their project won't get our dev dependencies the linter is telling you that they also won't have the assets-webpack-plugin.

The dev dependencies in Encore are used for optional things only (for instance if you use it with vue.js you'll have to add the loader manually), in this case we check that the dependency is present before requiring it (see notifier.js for instance).

For this plugin it makes more sense to add it to normal dependencies, so people will automatically get it when they install Encore.

Okay I will move it. Thank you!

Lyrkan · 2018-10-15T12:39:24Z

lib/plugins/entry-files-manifest.js

+function cleanFilePaths(assets) {
+    let tmpAssets = assets;
+    for (let asset in tmpAssets) {
+        if ('_' === asset.substr(0, 1)) {


Is that only to remove _tmp_shared_entry?

If yes it would probably be better to keep const sharedEntryTmpName = require('../utils/sharedEntryTmpName') and use that constant instead.

Lyrkan · 2018-10-15T12:41:22Z

lib/plugins/entry-files-manifest.js

+const AssetsPlugin = require('assets-webpack-plugin');
+
+function cleanFilePaths(assets) {
+    let tmpAssets = assets;


const tmpAssets

Lyrkan · 2018-10-15T12:41:44Z

lib/plugins/entry-files-manifest.js

+
+function cleanFilePaths(assets) {
+    let tmpAssets = assets;
+    for (let asset in tmpAssets) {


for (const asset in tmpAssets)

Lyrkan · 2018-10-15T12:42:15Z

lib/plugins/entry-files-manifest.js

+            delete assets[asset];
+            continue;
+        }
+        for (let fileType in tmpAssets[asset]) {


for (const fileType in tmpAssets[asset])

test/functional.js

Lyrkan · 2018-10-15T12:48:06Z

test/functional.js

@@ -1362,7 +1360,7 @@ module.exports = {
                const config = createWebpackConfig('web/build', 'dev');
                config.addEntry('main', ['./css/roboto_font.css', './js/no_require', 'vue']);
                config.addEntry('other', ['./css/roboto_font.css', 'vue']);
-                config.setPublicPath('http://localhost:8080/build');
+                config.setPublicPath('http://localhost:8080/custom_prefix');


I'm a bit skeptic about that change... the fact that this test had a different string for the public path and the manifest key prefix seemed to be intended. That's probably related to the manifest-key-prefix-helper not being used anymore.

But having the public path doesn't feel wrong either... I'm not sure what's expected here (ping @weaverryan).

Hmm. This is a tough one. In my original version, I apparently designed it (I say "apparently" because this is what the test tells us, but it was something I was "debating" by myself for a bit) so that when the entrypoints.json file is dumped, if the user is using an absolute publicPath (with http://), that is NOT included in the entrypoints values. Instead, the user would use the value from entrypoints.json to make a second lookup in manifest.json.

I'm not sure that was the best idea. And so, the NEW implementation is better. Specifically: it looks like the new implementation always makes the value in entrypoints.json the final value - so there is no manifest.json lookup necessary. Do you guys see any problems with this? It would mean that nobody could use entrypoint.json to simply get the "local" paths of the needed CSS or JS files - it would always be the absolute path (with the http:// if they have one set). I'm trying to think if that's a problem...

Anyways, if we assume that the new implementation is better, then there is just one change to make here: change the publicPath BACK to http://localhost:8080/build because it's useful to set this path to something different than the manifestKeyPrefix (it's just a better test). Then, update the test below to look for http://localhost:8080/build in the values. Also, the description of this test would need to be updated, or 2 tests would need to be created. The new description would be something like:

Custom public path is used in entrypoints.json, but does not affect manifest.json

@weaverryan I updated the test to what I think you meant? not quite sure. Let me know if it needs more changes.

Lyrkan · 2018-10-15T12:49:23Z

test/functional.js

-                            css: ['custom_prefix/main~other.css']
+                            js: [
+                                'http://localhost:8080/custom_prefix/runtime.js'
+                                ,'http://localhost:8080/custom_prefix/vendors~main~other.js'


The linter doesn't check it but usually the comma is placed after each element of an array:

[ 'foo', 'bar', 'baz', ]

lib/plugins/entry-files-manifest.js

stof · 2018-10-16T14:13:12Z

lib/plugins/entry-files-manifest.js

+const AssetsPlugin = require('assets-webpack-plugin');
+
+function cleanFilePaths(assets) {
+    const tmpAssets = assets;


this does not create a copy of the data structure. It only create a separate reference to the same object. So AFAICT, this is useless here, and you could as well use assets all the time (the other solution is that it is broken)

Maybe something like this instead? (I didn't test it so there may be some errors)

const processedAssets = assets .filter((entrypoint, name) => name !== sharedEntryTmpName); .map(entrypoint => { return entrypoint.map(typeAssets => { if (!Array.isArray(typeAssets)) { typeAssets = [typeAssets]; } return typeAssets.map(buildPath => buildPath.replace(/^\//g, '')); }); }); return JSON.stringify(processedAssets);

It would also put everything in an array like the previous code, even if there is only one file.

I tried to do the fancy snippet you had but it came out with filter is not a function. assets is a json object when it comes in. since I am doing dynamic keys that is why it looks like I am using an array when it is a json object. I checked the object prototype and I dont see any really helpful methods. I can try implementing them my self in some helper module. I dont know how fancy we want to go with it.

I also removed this variable and it now operates directly with the object that is passed in.

Yeah, nevermind, you'd have to filter Object.keys(assets) first and then rebuild the object... not sure that's worth it if directly mutating assets isn't an issue.

I think mutating it is fine if things work. But then, we should not create a tmpAssets variable, which make people reading the code think we have 2 separate data structures (while we only have a second name for the same one)

stof · 2018-10-16T14:13:55Z

lib/plugins/entry-files-manifest.js

-const manifestKeyPrefixHelper = require('../utils/manifest-key-prefix-helper');
+const AssetsPlugin = require('assets-webpack-plugin');
+
+function cleanFilePaths(assets) {


the function name looks weird, as it is not only cleaning paths, but also stringifying the output to JSON.

Maybe processOutput(assets) instead?
It's a bit vague but it would match the name of the related option of the assets-webpack-plugin.

I have changed the function name

Lyrkan · 2018-10-17T07:06:25Z

lib/plugins/entry-files-manifest.js

+function processOutput(assets) {
+    for (const asset in assets) {
+        if (sharedEntryTmpName === asset) {
+            delete assets[asset];


Couldn't this be an unconditionnal delete assets[sharedEntryTmpName]; placed outside of the loop instead?

codayblue · 2018-10-18T16:52:42Z

@Lyrkan Is there anything else that needs to be taken care of in this PR?

Lyrkan · 2018-10-18T17:28:33Z

@codayblue I think everything is fine now :)

codayblue · 2018-10-18T19:43:45Z

Is there an way to have appveyor run again? I dont understand why it failed. If it is a concern. If not we can just leave it with the 1 check failed.

Lyrkan · 2018-10-18T20:14:46Z

Don't worry about AppVeyor, it happens to fail for no real reason sometimes (mostly because of timeouts during functional tests), which seems to be the case here.

weaverryan · 2018-10-19T19:34:26Z

lib/plugins/entry-files-manifest.js

+
+function processOutput(assets) {
+    if (assets.hasOwnProperty(sharedEntryTmpName)) {
+        delete assets[sharedEntryTmpName];


We now need to delete one other "magic: entry that may exist :) #409 (comment)

I will get this done.

Altered the function to take care of this issue.

@weaverryan Is the any other files we need to delete from the output?

weaverryan · 2018-10-19T19:35:10Z

lib/plugins/entry-files-manifest.js

+    if (assets.hasOwnProperty(sharedEntryTmpName)) {
+        delete assets[sharedEntryTmpName];
+    }
+    for (const asset in assets) {


Can we add a little comment about what this is doing / why?

And, if I understand it correctly, this gives us complete control over the output, right? Could we "correct" the output so that if there's only one entry, it's always an array? That would remove one of the slightly weird edge-cases that the bundle would need to handle.

true I never thought of that. Yes we could turn it into an array. I will add that to this PR.

I updated the function to now put the single instances into an array. I also out a comment above the loop.

@weaverryan Is there anything else we need to do to the output?

weaverryan

One last comment! Also, could you rebase and squash into 1 commit? I can also do it, but it hits a conflict, so it'll be safer if you do it (we can watch the tests pass). Thanks!

weaverryan · 2018-10-23T15:44:13Z

lib/plugins/entry-files-manifest.js

+            includeAllFileTypes: true,
+            entrypoints: true,
+            processOutput: processOutput
+        }),
        priority: PluginPriorities.EntryFilesManifestPlugin


Can you change this to be a new AssetsPlugin?

Updated test files, also partial implementation of removing the / in the build path Cleaned up some code and fixed some tests Made adjuments so that private directories dont get added to the entries list. Fixed the rest of the tests Alter eslint rule to allow assets webpack plugin Updated to fix some of the requested changes Move the assets plugin to required instead of dev. Removed the change to ESLint rules Cleaned up commas in test array Swapped out cleanFilePaths to processOutput. Also removed unused variable. Updated test Moved the shared temp check to out side the loop. Altered function to remove runtime.js when it is by it self Fixed tests after merging master Reverted changes to fix tests on remote Removed versioning from failing remote tests since we cant verify what version of vue users use Renamed plugin priority label from EntryFilesManifestPlugin to AssetsPlugin Alter the assets processing in entry file to convert one entry into an array of one entry. Added a comment to explain the function. Then as suggested removed a new temp file.

weaverryan · 2018-10-26T15:31:28Z

Thank you Cody! And welcome to being an Encore contributor :D

This PR was merged into the master branch. Discussion ---------- Implement assets webpack plugin This should solve issue #408 because it implements the plugin that was requested to be looked into. The changes that have happened by implementing this plugin is that the keys might not be in the file for a given entry. So lets say we have a main entry point that has JavaScript but no CSS, the CSS key will not be under that entry point. The other issue is that when a file type only has one file of a given file type it will not be in an array in the entrypoints.json file it will just be a string value linked to the property. Commits ------- 2f106e8 First iteration of of assets-webpack-plugin

Lyrkan requested changes Oct 15, 2018

View reviewed changes

stof reviewed Oct 16, 2018

View reviewed changes

stof mentioned this pull request Oct 16, 2018

Initial bundle setup & feature implementation symfony/webpack-encore-bundle#1

Merged

Lyrkan reviewed Oct 17, 2018

View reviewed changes

Lyrkan approved these changes Oct 18, 2018

View reviewed changes

weaverryan mentioned this pull request Oct 19, 2018

Add a copyFiles() method to the public API (using require.context) #409

Merged

weaverryan reviewed Oct 19, 2018

View reviewed changes

weaverryan reviewed Oct 23, 2018

View reviewed changes

weaverryan merged commit 2f106e8 into symfony:master Oct 26, 2018

Lyrkan mentioned this pull request Nov 5, 2018

Use ztoben/assets-webpack-plugin instead of creating our own entrypoints.json file #408

Closed

Implement assets webpack plugin #410

Implement assets webpack plugin #410

Conversation

codayblue commented Oct 14, 2018 • edited Loading

Lyrkan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lyrkan Oct 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lyrkan Oct 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codayblue Oct 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codayblue commented Oct 18, 2018

Lyrkan commented Oct 18, 2018

codayblue commented Oct 18, 2018 • edited Loading

Lyrkan commented Oct 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weaverryan Oct 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weaverryan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weaverryan commented Oct 26, 2018

codayblue commented Oct 14, 2018 •

edited

Loading

Lyrkan Oct 16, 2018 •

edited

Loading

Lyrkan Oct 17, 2018 •

edited

Loading

codayblue Oct 17, 2018 •

edited

Loading

codayblue commented Oct 18, 2018 •

edited

Loading

weaverryan Oct 19, 2018 •

edited

Loading